Switch to proxy URL to handle API requests to demo, #PG-5029#876
Conversation
| window.onload = function () { | ||
| let pluginSpec = {{ pluginSpecJson|raw }}; | ||
| let pluginSpecJson = {{ pluginSpecJson|raw }}; | ||
| let proxyBaseUrl = window.location.origin + '/demo-proxy'; |
There was a problem hiding this comment.
| let proxyBaseUrl = window.location.origin + '/demo-proxy'; | |
| let proxyBaseUrl = '/demo-proxy'; |
There was a problem hiding this comment.
Made that change, it also collapses the server field down to this
I changed it to 'demo' rather than 'demo-proxy', proxy felt too technical for a user facing page.
|
|
||
| $proxiedResponse = DemoProxy::get($targetUrl, $request->getHeaderLine('Authorization')); | ||
|
|
||
| $response->getBody()->write($proxiedResponse['body']); |
There was a problem hiding this comment.
we should add a try/catch here
There was a problem hiding this comment.
Done, I also realised we return errors with a 200 OK? At the moment it doesn't break anything in Swagger, technically we could catch in the proxy and throw something else
| }); | ||
|
|
||
| $app->get('/demo-proxy/{path:.*}', function (Request $request, Response $response, $args) { | ||
| $path = ltrim($args['path'] ?? '', '/'); |
There was a problem hiding this comment.
main concern is that it now becomes a proxy to query demo.matomo.cloud without any CORS error
We can add a check that requestURL path should be index.php and Module should be API along with CSRF token.
There was a problem hiding this comment.
Have added some URL validation so only the API can be queried.
Demo is a public instance, are we exposing anything that isn't already available with this proxy? For the CSRF token, we don't have a session on developer docs, so not sure where we can store that
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
Is this correct ?
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
left few comments
@AltamashShaikh That's the new proxy endpoint that swagger will make requests against, do you think it should still show demo.matomo.cloud? I might check if I can change the text but not the URL |
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
@lachiebol Good to add some tests with the help of AI
|
@AltamashShaikh Sorry, just got around to this, but we don't have PHPUnit setup at all, and no testing workflow for this. Do we want to introduce testing pipelines for developer docs? |
@lachiebol Create a follow-up ticket, we should add it at the end, nothing is needed now. |
2 participants
Description
Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.
Checklist
Review